Skip to content

Conversation

@JacksonYao287
Copy link
Contributor

@JacksonYao287 JacksonYao287 commented Aug 3, 2021

What changes were proposed in this pull request?

make balancer HA aware

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-5253

How was this patch tested?

unit test

@JacksonYao287
Copy link
Contributor Author

@lokeshj1703 @ChenSammi @siddhantsangwan PTAL! if the logics looks good , i will add more unit test in additional commit, thanks

@JacksonYao287 JacksonYao287 changed the title make balancer HA aware HDDS-5253. make balancer HA aware Aug 3, 2021
Copy link
Contributor

@lokeshj1703 lokeshj1703 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacksonYao287 Thanks for working on this PR! The changes look good. I have few minor comments inline.

@JacksonYao287
Copy link
Contributor Author

i have updated the PR according the comments, @lokeshj1703 @ChenSammi @siddhantsangwan please take a look!

@lokeshj1703
Copy link
Contributor

@JacksonYao287 Can you also add a UT with HA to test the consistency of move?

@JacksonYao287
Copy link
Contributor Author

Can you also add a UT with HA to test the consistency of move?

@lokeshj1703 , sure, thanks, if the logic looks good to you now, i will add UT in a new commit

@JacksonYao287
Copy link
Contributor Author

@lokeshj1703 I have added UT and integration tests for move HA, PTAL!

@JacksonYao287
Copy link
Contributor Author

CI failures seems not caused by this patch, i will merge mater branch again after they are fixed! #2420

Copy link
Contributor

@lokeshj1703 lokeshj1703 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacksonYao287 Thanks for updating the PR! I have few minor comments. +1 o.w.

@lokeshj1703
Copy link
Contributor

Also we will need to make sure that balancer is always restarted on leader with the new configuration. This would be a separate PR.

@JacksonYao287
Copy link
Contributor Author

Also we will need to make sure that balancer is always restarted on leader with the new configuration. This would be a separate PR.

sure , will do this

@lokeshj1703
Copy link
Contributor

@JacksonYao287 We will also need to add timeout for move with the leader changes. After a leader change the move will be reset so I think we will need to handle the timeouts as well. It might require some thought. Please see if you would like to address it in this PR.
Let's also rename jira and PR to sth like Support move with HA.

@JacksonYao287 JacksonYao287 changed the title HDDS-5253. make balancer HA aware HDDS-5253. Support container move HA Aug 16, 2021
@JacksonYao287
Copy link
Contributor Author

We will also need to add timeout for move with the leader changes. After a leader change the move will be reset so I think we will need to handle the timeouts as well. It might require some thought. Please see if you would like to address it in this PR.

thanks @lokeshj1703 for pointing out this. i think it is better off addressing it in a new jira, will do it later

@lokeshj1703 lokeshj1703 merged commit 05c7a98 into apache:master Aug 16, 2021
@lokeshj1703
Copy link
Contributor

@JacksonYao287 Thanks for the contribution! I have committed the PR to master branch.

@JacksonYao287 JacksonYao287 deleted the HDDS-5253 branch August 16, 2021 11:18
@JacksonYao287
Copy link
Contributor Author

thanks @lokeshj1703 for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants